-
Notifications
You must be signed in to change notification settings - Fork 110
swap, contracts, vendor: move to ERC20 #1964
Conversation
note: while tests pass not all logic has been updated yet
…, remove backend as input parameter
…-amount, add flag no-deposit added test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general great work, the core of it is ready to merge.
Just a couple of questions, check them please.
And some issues:
- use the "natural" zero value of
big.Int
instead of creating a new instance all the time - I often get remarks that I comment too much, but I maintain better too much comments than too little. Generally, don't ignore return values, and if the test really can, comment why.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small nitpick but approving.
Generally though, for future PRs, I see a lot if ignored return values in the tests. It indeed looks like they are not needed for the tests in question, which is why I approve, but it is generally a rejected practice in the project, and should be avoided whenever possible.
… indicating in prompt that 0 means not depositing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i've left some questions in order to better understand these changes before giving my approvial.
besides these, i'd like to ask: why did we go for a mintable ERC20 instead of a regular one?
finally: please create an issue (if there isn't one already) for the Ropsten factory update in case this is still pending.
heads-up: it seems the vendoring should be checked as well |
initialDepositAmount
todepositAmount
skipDeposit
flag